✅ Cross-SDK Consistency Review: PASSED

I've reviewed PR #523 for consistency across all four SDK implementations (Node.js/TypeScript, Python, Go, and .NET). This PR demonstrates exemplary cross-SDK consistency in implementing the feature to auto-exclude built-in tools overridden by user-registered tools.

Feature Implementation Consistency

All four SDKs implement the same core functionality with appropriate language-specific idioms:

1️⃣ Helper Function Pattern

• Node.js: mergeExcludedTools() - camelCase function ✅
• Python: Inline implementation using dict.fromkeys() for deduplication ✅
• Go: mergeExcludedTools() - camelCase (unexported/private) ✅
• .NET: MergeExcludedTools() - PascalCase (internal static) ✅

All implementations properly deduplicate and merge tool names with existing excludedTools.

2️⃣ Integration Points

All SDKs apply the merge logic in both:

• createSession / create_session / CreateSession / CreateSessionAsync ✅
• resumeSession / resume_session / ResumeSessionWithOptions / ResumeSessionAsync ✅

3️⃣ Unit Test Coverage

Each SDK includes comprehensive unit tests covering:

• ✅ Adding tool names to excludedTools
• ✅ Deduplication with existing excludedTools
• ✅ No-op when no tools provided
• ✅ Resume session path

Test locations:

• Node.js: nodejs/test/client.test.ts (4 tests in describe("excludedTools merging with config.tools"))
• Python: python/test_client.py (TestExcludedToolsFromRegisteredTools class, 4 tests)
• Go: go/client_test.go (TestMergeExcludedTools with 4 sub-tests)
• .NET: dotnet/test/MergeExcludedToolsTests.cs (5 tests)

4️⃣ E2E Test Coverage

All SDKs include E2E tests verifying the feature works end-to-end:

• ✅ Node.js: nodejs/test/e2e/tools.test.ts - "overrides built-in tool with custom tool"
• ✅ Python: python/e2e/test_tools.py - test_overrides_built_in_tool_with_custom_tool
• ✅ Go: go/internal/e2e/tools_test.go - "overrides built-in tool with custom tool"
• ✅ .NET: dotnet/test/ToolsTests.cs - Overrides_Built_In_Tool_With_Custom_Tool

All E2E tests use the same approach: register a custom grep tool and verify it returns CUSTOM_GREP_RESULT instead of calling the built-in.

5️⃣ Documentation Consistency

All four READMEs include an identical "Overriding Built-in Tools" subsection with:

• ✅ Clear explanation of the feature
• ✅ Language-appropriate code example
• ✅ Consistent messaging about automatic behavior

6️⃣ Test Scenarios & Snapshots

The PR includes cross-language example implementations in test/scenarios/tools/tool-overrides/:

• ✅ TypeScript implementation
• ✅ Python implementation
• ✅ Go implementation
• ✅ C# implementation
• ✅ Shared verification script (verify.sh)
• ✅ Shared snapshot (test/snapshots/tools/overrides_built_in_tool_with_custom_tool.yaml)

Language-Specific Differences (All Appropriate)

The only differences are idiomatic language conventions, which are correctly applied:

1. Naming conventions: camelCase (TS/Python), camelCase for private/PascalCase for public (Go), PascalCase (.NET)
2. Visibility modifiers:
   • Node.js: module-private function
   • Python: inline (no separate function needed)
   • Go: unexported function (lowercase)
   • .NET: internal static method with InternalsVisibleTo for testing
3. Deduplication approach:
   • Node.js: Set constructor
   • Python: dict.fromkeys()
   • Go: map[string]bool for seen tracking
   • .NET: LINQ Union()

All approaches are idiomatic and achieve the same result.

Conclusion

🎉 This PR maintains perfect feature parity across all four SDK implementations. The feature is:

• Implemented consistently with appropriate language idioms
• Thoroughly tested with both unit and E2E tests
• Well-documented in all READMEs
• Includes comprehensive example scenarios

No cross-SDK consistency issues found. Excellent work! 👏

AI generated by SDK Consistency Review Agent

✅ Cross-SDK Consistency Review: PASSED

This PR demonstrates excellent cross-language consistency in implementing the auto-exclude feature for user-registered tools that override built-ins. All four SDK implementations maintain feature parity with properly adapted API patterns.

Summary of Changes

The PR adds functionality across all SDKs to automatically merge user-registered tool names into excludedTools when calling session.create or session.resume, ensuring custom tools override CLI built-ins with the same name.

Consistency Analysis

✅ Core Implementation Pattern

All SDKs implement the same logic with language-appropriate patterns:

✅ Integration Points

Consistently applied in both createSession/create_session/CreateSession/CreateSessionAsync and resumeSession/resume_session/ResumeSessionWithOptions/ResumeSessionAsync across all SDKs.

✅ Test Coverage

All SDKs include comprehensive unit and E2E tests:

Unit Tests (testing the merge logic):

• Node.js: 4 tests in nodejs/test/client.test.ts
• Python: 4 tests in python/test_client.py (TestExcludedToolsFromRegisteredTools)
• Go: 4 sub-tests in go/client_test.go (TestMergeExcludedTools)
• .NET: 5 tests in dotnet/test/MergeExcludedToolsTests.cs

E2E Tests (testing actual override behavior):

• Node.js: nodejs/test/e2e/tools.test.ts - "overrides built-in tool with custom tool"
• Python: python/e2e/test_tools.py - test_overrides_built_in_tool_with_custom_tool
• Go: go/internal/e2e/tools_test.go - "overrides built-in tool with custom tool"
• .NET: dotnet/test/ToolsTests.cs - Overrides_Built_In_Tool_With_Custom_Tool

All E2E tests use the same pattern: register a custom grep tool and verify it returns CUSTOM_GREP_RESULT.

✅ Documentation

Identical "Overriding Built-in Tools" subsections added to all four READMEs with language-appropriate code examples, consistent messaging, and proper placement in the "Custom Tools" section.

✅ Naming Conventions

Properly adapted to language idioms:

• Node.js: excludedTools (camelCase)
• Python: excluded_tools (snake_case)
• Go: ExcludedTools (PascalCase - public field)
• .NET: ExcludedTools (PascalCase)

Edge Cases Handled Consistently

All implementations correctly handle:

• ✅ Null/empty tools list (no-op)
• ✅ Null/empty excludedTools (returns tool names)
• ✅ Deduplication when tool names appear in both lists
• ✅ Both create and resume session flows

No Issues Found

This PR maintains excellent cross-SDK consistency. The feature is implemented uniformly across all languages with appropriate adaptations for each ecosystem's conventions and idioms. Great work! 🎉

AI generated by SDK Consistency Review Agent

✅ Cross-SDK Consistency Review: PR #523

I've completed a comprehensive review of this PR across all four SDK implementations (Node.js, Python, Go, .NET). Overall, this PR demonstrates excellent cross-SDK consistency with proper feature parity across all languages.

📊 Summary

✅ Consistent Implementation Patterns

Each SDK correctly implements the auto-exclusion logic with language-appropriate idioms:

Node.js (nodejs/src/client.ts)

function mergeExcludedTools(excludedTools?: string[], toolNames?: string[]): string[] | undefined {
    if (!toolNames || toolNames.length === 0) return excludedTools;
    return [...new Set([...(excludedTools ?? []), ...toolNames])];
}

• ✅ Uses Set for deduplication (idiomatic JS/TS)
• ✅ Applied in both createSession (lines 237, 253) and resumeSession (line 275)

Python (python/copilot/client.py)

excluded_tools = list(dict.fromkeys(excluded_tools + tool_names))

• ✅ Uses dict.fromkeys() for order-preserving deduplication (idiomatic Python 3.7+)
• ✅ Applied in both create_session (lines 498-503) and resume_session (lines 679-684)
• ⚠️ Minor note: Unlike the other SDKs, Python inlines the logic rather than using a dedicated helper function. This is acceptable as it's only 1 line, but creating a _merge_excluded_tools() helper would improve consistency with the other SDKs and DRY principles.

Go (go/client.go)

func mergeExcludedTools(excludedTools []string, tools []Tool) []string {
    // map-based deduplication with order preservation
}

• ✅ Uses map[string]bool for deduplication (idiomatic Go)
• ✅ Applied in both CreateSession (line 471) and ResumeSessionWithOptions (line 568)

.NET (dotnet/src/Client.cs)

internal static List(string)? MergeExcludedTools(List(string)? excludedTools, List(AIFunction)? tools) {
    return excludedTools.Union(toolNames).ToList();
}

• ✅ Uses LINQ Union() for deduplication (idiomatic C#)
• ✅ Applied in both CreateSessionAsync (line 392) and ResumeSessionAsync (line 483)
• ✅ Added InternalsVisibleTo for test access

✅ Test Coverage Consistency

All four SDKs include comprehensive test coverage:

Unit Tests

• Node.js (nodejs/test/client.test.ts): 4 test cases covering create/resume paths and deduplication
• Python (python/test_client.py): 4 test cases with equivalent coverage
• Go (go/client_test.go): 5 test cases including edge cases
• .NET (dotnet/test/MergeExcludedToolsTests.cs): 5 test cases in dedicated test file

E2E Tests

• All 4 SDKs include E2E tests verifying the grep override scenario
• Uses shared YAML snapshot: test/snapshots/tools/overrides_built_in_tool_with_custom_tool.yaml

✅ Documentation Consistency

All four SDK READMEs include an identical "Overriding Built-in Tools" subsection with:

• Clear explanation of the auto-exclusion behavior
• Language-appropriate code examples
• Placement in the Tools section

Additionally, a multi-language sample project was added in test/scenarios/tools/tool-overrides/ with implementations in TypeScript, Python, Go, and C#.

🎯 API Design Consistency

The feature maintains proper naming conventions across languages:

All naming follows the established language conventions (camelCase for JS/TS, snake_case for Python, PascalCase for Go/C# public APIs).

💡 Minor Enhancement Suggestion (Non-Blocking)

Python only: Consider extracting a _merge_excluded_tools() helper function to match the pattern used in the other three SDKs. Current inline approach works fine, but a dedicated function would:

• Improve testability (can unit test in isolation)
• Match the architectural pattern of Go/.NET/Node.js
• Make the code easier to maintain if logic needs to be enhanced

Example:

def _merge_excluded_tools(excluded_tools: list[str] | None, tool_names: list[str] | None) -> list[str] | None:
    if not tool_names:
        return excluded_tools
    return list(dict.fromkeys((excluded_tools or []) + tool_names))

🎉 Conclusion

This PR is an exemplary model of cross-SDK consistency. The feature:

• ✅ Has identical behavior across all 4 languages
• ✅ Uses language-appropriate idioms and conventions
• ✅ Includes comprehensive test coverage everywhere
• ✅ Has consistent documentation
• ✅ Handles edge cases uniformly (empty lists, null/undefined, deduplication)

No blocking consistency issues found. The minor Python suggestion above is purely optional for architectural alignment.

Great work maintaining feature parity! 🚀

AI generated by SDK Consistency Review Agent

Waiting on runtime change to land in separate PR.